Skip to content

fix(mocks): WasCalled/WasNeverCalled assertions via [GenerateAssertion] for all ICallVerification types#6176

Merged
thomhurst merged 2 commits into
mainfrom
feature/mock-assertion-verify-fix
Jun 7, 2026
Merged

fix(mocks): WasCalled/WasNeverCalled assertions via [GenerateAssertion] for all ICallVerification types#6176
thomhurst merged 2 commits into
mainfrom
feature/mock-assertion-verify-fix

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Summary

  • Replace hand-written WasCalledAssertion/WasNeverCalledAssertion classes and IAssertionSource<T> extension overloads with generic [GenerateAssertion] methods constrained to ICallVerification
  • Fixes await Assert.That(mock.Method(Any())).WasCalled(Times.Once) not compiling for generated mock call types (MockMethodCall<T>, PropertyMockCall<T> struct, VoidMockMethodCall, PropertySetterMockCall<T>)
  • WasCalled(Times) and WasNeverCalled remain direct extension methods so the non-assertion form mock.Add(1, 2).WasCalled(Times.Once) keeps working

Test coverage

New MockAssertionExtensionsTests (32 tests, all through the assertion pipeline):

  • Positive: WasCalled() shorthand, all Times variants incl. boundaries (Once/Exactly/AtLeast/AtMost/Between/Never), WasNeverCalled(), arg-specific counting, Any() matcher, void methods, property getters (struct PropertyMockCall<T> — exercises the generic constraint path), property setters via .Setter and .Set(value)
  • Negative: each Times variant with wrong count asserts AssertionException with Mock verification failed / called N time(s) message, WasNeverCalled after call, non-matching args, null ICallVerification target

Also cleaned up VerificationTests.Greet_Returns_Configured_Value (stale 'does not compile' comment removed; result now asserted).

Test results

  • MockAssertionExtensionsTests: 32/32 pass (net10.0)
  • VerificationTests: 15/15 pass (net10.0)

…eAssertion]

Replace hand-written WasCalledAssertion/WasNeverCalledAssertion classes and
IAssertionSource extension overloads with generic [GenerateAssertion] methods
constrained to ICallVerification. Fixes Assert.That(mock.Method(Any())).WasCalled(...)
failing to compile for generated mock call types (MockMethodCall<T>,
PropertyMockCall<T> struct, VoidMockMethodCall).

Add MockAssertionExtensionsTests: 32 positive/negative tests covering the
assertion pipeline across all Times variants, arg matching, void methods,
property getters/setters, and null verification targets.
@codacy-production

codacy-production Bot commented Jun 7, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 32 complexity

Metric Results
Complexity 32

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The core refactoring is the right approach — replacing hand-written WasCalledAssertion/WasNeverCalledAssertion classes with [GenerateAssertion] generic methods is exactly what this pattern was designed for, and the fix for struct types (PropertyMockCall<T>) is genuine. Test coverage is thorough.

Two issues to address:


Bug: Inner exception is silently dropped

MockAssertionExtensions.csWasCalled<T> and WasNeverCalled<T>

// New code:
catch (MockVerificationException ex)
{
    return AssertionResult.Failed(ex.Message);  // inner exception lost
}

The deleted WasCalledAssertion was:

return Task.FromResult(AssertionResult.Failed(ex.Message, ex));  // passed ex

AssertionResult.Failed(string, Exception?) exists (I checked). Dropping ex means anyone inspecting the assertion failure's inner exception (e.g., a debugger, a diagnostics tool, TUnit's own failure reporting) gets null instead of the real exception. This is a silent regression. Both catch blocks in the new code need ex threaded through:

catch (MockVerificationException ex)
{
    return AssertionResult.Failed(ex.Message, ex);
}

Design issue: IGreeter declared inside VerificationTests shadows the top-level one

VerificationTests.cs lines 253–256

There is already a top-level public interface IGreeter in BasicMockTests.cs (same TUnit.Mocks.Tests namespace). The PR adds a second, identically-shaped nested IGreeter inside the VerificationTests class.

Inside VerificationTests, all unqualified IGreeter references — including the pre-existing Verify_String_Method_On_Different_Interface test — will silently resolve to the new nested type rather than the top-level one. The code compiles and works because the signatures are identical, but this is a shadowing trap: if either definition ever diverges, the test will start testing the wrong type without a compile error.

Better approach: reuse the existing top-level IGreeter; it has the same Greet(string name) signature and is already used by other tests in the file via BasicMockTests.IGreeter. No new type is needed.


Minor: WasCalled() shorthand expectation message is vague

[GenerateAssertion(ExpectationMessage = "to have been called")]
public static AssertionResult WasCalled<T>(T verification) where T : ICallVerification

This assertion checks Times.AtLeastOnce, so when it fails the error reads: "Expected X to have been called but was called 0 time(s)". "to have been called at least once" would be more precise and match the other Times-based messages in style.


Good things worth noting

  • Eliminating WasCalledAssertion/WasNeverCalledAssertion is the right simplification — the source generator handles all that boilerplate.
  • Using a generic constraint where T : ICallVerification instead of IAssertionSource<ICallVerification> is the correct fix for the struct types.
  • The 32-test suite covers boundary cases, all Times variants, and all mock call types comprehensively.
  • WasCalled(Times) and WasNeverCalled are correctly kept as extension methods so the non-assertion form mock.Add(1, 2).WasCalled(Times.Once) continues to work.

- Preserve MockVerificationException as inner exception in AssertionResult.Failed
- Remove nested IGreeter in VerificationTests that shadowed the top-level interface
- Clarify WasCalled() expectation message to "to have been called at least once"
@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed all three review items in 4df31cc:

  • Inner exception: both catch blocks now call AssertionResult.Failed(ex.Message, ex), matching the deleted assertion classes' behavior.
  • Nested IGreeter: removed — verified the shadowing also affected the pre-existing Verify_String_Method_On_Different_Interface test; both tests now use the top-level IGreeter.
  • Expectation message: WasCalled() now uses "to have been called at least once".

Ran VerificationTests + MockAssertionExtensionsTests locally: 47/47 passed in both source-gen and reflection modes.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up Review — Previous items all addressed

All three items from the first review round have been correctly fixed in 4df31cc:

  • Both catch blocks now pass ex to AssertionResult.Failed(ex.Message, ex) — inner exception preserved.
  • The nested IGreeter shadow has been removed — VerificationTests and MockAssertionExtensionsTests both use the top-level IGreeter from BasicMockTests.cs.
  • WasCalled() shorthand expectation message is now "to have been called at least once".

One minor cleanup remaining

MockAssertionExtensions.cs still has:

using System.Runtime.CompilerServices;

This was needed for [CallerArgumentExpression] on the old WasCalled(Times, string?) overloads. Those overloads were deleted in this PR and CallerArgumentExpression is no longer referenced anywhere in the file. The import is now unused and should be removed.


Everything else looks good

  • The [GenerateAssertion] approach correctly handles the where T : ICallVerification constraint, fixing the struct type (PropertyMockCall<T>) limitation.
  • WasCalled(Times) and WasNeverCalled() are correctly preserved as plain extension methods so the non-assertion form mock.Add(1, 2).WasCalled(Times.Once) continues to work without going through the assertion pipeline.
  • IPropertyHolder being nested inside MockAssertionExtensionsTests is fine — it's a test-only interface specific to that class.
  • The 32-test suite is comprehensive and well-structured.

The stale import is the only remaining item. Happy to approve once that's cleaned up, or it can be left for a follow-up — it's low severity.

@thomhurst thomhurst enabled auto-merge (squash) June 7, 2026 16:30
@thomhurst thomhurst merged commit 0970925 into main Jun 7, 2026
16 of 17 checks passed
@thomhurst thomhurst deleted the feature/mock-assertion-verify-fix branch June 7, 2026 17:35
This was referenced Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant